Implementing result pipe drainage on cancellation#25981
Open
Copilot wants to merge 9 commits into
Open
Conversation
Copilot created this pull request from a session on behalf of
eleanorjboyd
May 29, 2026 21:26
View session
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The pure event-driven drain in this PR can hang the adapter's finally block forever if reader.onClose never fires - which happens in CI on the segfault/coverage/large-workspace execution tests (abnormal subprocess exit, platform-specific named-pipe quirks). Add awaitDeferredWithTimeout helper and use it in both unittest and pytest execution adapters' finally blocks. By the time we hit finally, runTestsNew has already awaited the subprocess 'exit' event, so a few seconds is plenty for any buffered pipe data to flush. The timeout acts purely as a backstop - the normal path still resolves promptly via reader.onClose. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the test execution adapters’ result-pipe lifecycle so cancellation no longer detaches the result listener prematurely, allowing buffered results to drain and preventing hangs by adding a timeout backstop.
Changes:
- Stop resolving the “server closed” deferred on cancellation so the result pipe can drain naturally via
reader.onClose. - Add
awaitDeferredWithTimeout()+RESULT_PIPE_DRAIN_TIMEOUT_MSand use it in pytest/unittest adapters to avoid indefinite waits. - Add unit tests covering drain-on-cancel behavior for
startRunResultNamedPipeand timeout behavior.
Show a summary per file
| File | Description |
|---|---|
| src/client/testing/testController/common/utils.ts | Adds drain timeout constant + awaitDeferredWithTimeout, and adjusts result-pipe disposal behavior to be close-driven. |
| src/client/testing/testController/unittest/testExecutionAdapter.ts | Stops resolving the “server closed” deferred on cancellation and uses timeout-backed waiting in finally. |
| src/client/testing/testController/pytest/pytestExecutionAdapter.ts | Uses timeout-backed waiting for result-pipe closure in finally. |
| src/test/testing/testController/utils.unit.test.ts | Adds unit tests validating drain-on-cancel semantics and timeout behavior. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Use traceInfo instead of console.log in unittest cancellation handler (consistent with other test controller adapters; routes through the extension output channel). - Clarify awaitDeferredWithTimeout log message: the underlying deferred is not actually resolved on timeout, the helper just stops waiting. - Fill in required ExecutionTestPayload fields (status, error) in the test helper so emitted messages match the real runner schema. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
dmitrivMS
approved these changes
Jun 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull request created by AI Agent